Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New hooks API (replaces monkey-patching for currency) #1683

Merged
merged 8 commits into from
Oct 30, 2017

Conversation

snapwich
Copy link
Collaborator

Type of change

  • Feature
  • Refactoring (no functional changes, no api changes)

Description of change

This pull-request provides a new API for adding hooks into the Prebid core functionality. By wrapping code paths with createHook, Prebid.js modules (and possibly external code, if we decide to expose hooks) can extend the functionality of core code through various models provided in hook.js. It follows in the spirit of https://github.com/webpack/tapable (the library webpack uses for adding extensibility). However, rather than adding extensibility for class methods through mixins (like tapable), I opted for adding extensibility to plain function since much of Prebid core is functional in nature.

So far the only extensibility options for hooks that I added are sync and asyncSeries, but other could be added if needed as seen within the tapable library's readme.

This replaces the monkey-patching that currently takes place in the currency module. Once merged, the 1.0 branch can be updated to put the appropriate hook in place for currency to still function properly.

The hook API in this pull-request being used for bidmanager.addBidResponse in pre 1.0 is like the following:

// bidmanager.js
import { createHook } from 'src/hook.js';

exports.addBidResponse = createHook('asyncSeries', function(adUnitCode, bid) {
  // add bid response stuff
});

This allows any module that wants to extend bidmanager.addBidResponse functionality to import the method and add a hook.

// some module
import { addBidResponse } from 'src/bidmanager.js';
addBidResponse.addHook(function(adUnitCode, bid, next) {
  // modify bid or something
  next(adUnitCode, bid); // continue execution of further hooks, or finally call addBidResponse if this is only hook
}, 50); // `addHook` allows for optional parameter specifying priority.  Higher priority hooks will run first

For making this compatible with 1.0 we need only to wrap createHook around whatever method we want extended (I think a new addBidResponse w/o the bidmanager) and expose the hook in a way that modules can get access to them . Either export the function in the new auctionManager (or wherever) or provide a global hook name as a third parameter to createHook.

Example:

// auction.js
adaptermanager.callBids(_adUnits, bidRequests, createHook('asyncSeries', addBidResponse.bind(this), 'addBidResponse'), done.bind(this)); // global hook named 'addBidResponse' created

// some module
import { hooks } from 'src/hook.js';

hooks['addBidResponse'].addHook(function() { ... });

With this new API we could hopefully start creating more hook-points in Prebid.js core which allows for more code to be moved into modules for optional inclusion.

@dbemiller
Copy link
Contributor

This is much more elegant than monkey-patching... but I'm not sure it solves the 1.0 problem.

The core problem in 1.0 is that the addBidResponse function is no longer global. Instead, each auction instance makes its own addBidResponse.

This means that the currency module (which is global) can't get a reference to it, so it would be impossible to call addHook using the method reference directly.

I think the global API (hooks[name].addHook() and hooks[name].createHook()) has a similar, related problem.

The first auction runs createHook, which makes a hooks['addBidResponse']. The currency module adds its decorator. Then a second auction gets created, which sets hooks['addBidResponse'] to a new object. The currency module would only get applied once.

I haven't looked into the 1.0 branch closely enough yet... but I think there's also a race condition here. If a pub does something like:

pbjs.setConfig(currencyStuff);
setTimeout(function() {
  pbjs.callBids();
}, 5000)

then there's a good chance that initCurrency will run before createHook ever gets created. In that case, hooks['addBidResponse'].addHook() will end in an undefined reference error.

@snapwich
Copy link
Collaborator Author

So I attempted a few methods to solve the auction instance problems w/ 1.0 such as allowing callbacks to hooks and having addBidResponse be one, that way it can keep its closure context and we could have a global hook.

However, the method that I think makes the most sense is fixing addBidResponse. In its current state it requires a lot of context to operate, and that context is grabbed from its outside scope creating a closure with a bunch of implicit dependencies. It's very hard to debug and follow. I suggest we make instance dependencies explicit through use of this, such as this._bidderRequest.find as opposed to _bidderRequest.find. This allows us to set the appropriate instance context for each callback passed to the adapters. This would also allow us to move addBidResponse to a more global scope to create a hook.

I updated this pull-request to be more accepting of context passing and binding for hook functions.

The changes to auction.js would look similar to this:

// outer module scope
export const addBidResponse = createHook('asyncSeries', function addBidResponse(adUnitCode, bid) {
  ...
  function addBidToAuction() {
    events.emit(CONSTANTS.EVENTS.BID_RESPONSE, bid);
    this.addBidReceived(bid); // instance functions and properties referenced by this
  }
})

// callBids auction instance method could go back to the following
function callBids() {
  ...

  adaptermanager.callBids(_adUnits, bidRequests, addBidResponse.bind(this), done.bind(this));
};

Basically we'll be treating addBidResponse more like a public prototype method (even though we don't use classes) than an inner private closure function, which I think is ideal. Thoughts?

Copy link
Contributor

@dbemiller dbemiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will work, as a general strategy... but there are a few bugs which should probably be fixed before merging.

hook.fn.apply(this, args);
});
},
asyncSeries: function(...args) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure exactly what the issue(s) are... but I uncovered some weird bugs in asyncSeries hooks.

The following test passes, as expected:

    it('should allow context to be passed to hooks, but keep bound contexts', () => {
      let context;
      let fn = function() {
        context = this;
      };

      let boundContext1 = {};
      let calledBoundContext1;
      let hook1 = function(next) {
        calledBoundContext1 = this;
        next()
      }.bind(boundContext1);

      let hookFn = createHook('asyncSeries', fn);
      hookFn.addHook(hook1);

      hookFn();

      expect(calledBoundContext1).to.equal(boundContext1);
    });

However, it begins failing if you add a second hook, like so:

   it('should allow context to be passed to hooks, but keep bound contexts', () => {
      let context;
      let fn = function() {
        context = this;
      };

      let boundContext1 = {};
      let calledBoundContext1;
      let hook1 = function(next) {
        calledBoundContext1 = this;
        next()
      }.bind(boundContext1);

      let boundContext2 = {};
      let calledBoundContext2;
      let hook2 = function(next) {
        calledBoundContext2 = this;
        next()
      }.bind(boundContext2);

      let hookFn = createHook('asyncSeries', fn);
      hookFn.addHook(hook1);

      // let newContext = {};
      // hookFn = hookFn.bind(newContext);
      hookFn();

      // expect(context).to.equal(newContext);
      expect(calledBoundContext1).to.equal(boundContext1);
      expect(calledBoundContext2).to.equal(boundContext2);
    });

It also fails with a single hook if the hookable function is bound:

    it('should allow context to be passed to hooks, but keep bound contexts', () => {
      let context;
      let fn = function() {
        context = this;
      };

      let boundContext1 = {};
      let calledBoundContext1;
      let hook1 = function(next) {
        calledBoundContext1 = this;
        next()
      }.bind(boundContext1);

      let hookFn = createHook('asyncSeries', fn);
      hookFn.addHook(hook1);

      let newContext = {};
      hookFn = hookFn.bind(newContext);
      hookFn();

      expect(context).to.equal(newContext);
      expect(calledBoundContext1).to.equal(boundContext1);
    });

The error messages aren't very helpful... just TypeError: Cannot read property 'replace' of undefined, with no line number.

Copy link
Collaborator Author

@snapwich snapwich Oct 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first test case you list I think is failing because you never added the second hook. I think adding hookFn.addHook(hook2); should fix it.

The second test case is a bug. I wasn't properly maintaining the this reference in the asyncSeriesNext function. I updated the code to use a fat-arrow function to keep the proper this and updated the test w/ your use case and it seems to be passing now. Thanks.

originalBidResponse = bidmanager.addBidResponse;
bidmanager.addBidResponse = addBidResponseDecorator(bidmanager.addBidResponse);
}
bidmanager.addBidResponse.addHook(addBidResponseHook, 100);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any value in setting a priority when no hooks compete yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. If others add hooks later this one will still run first. I specifically wanted currency running first as it buffers responses (if the currency file isn't back yet).

@snapwich
Copy link
Collaborator Author

updated

@dbemiller dbemiller removed their assignment Oct 24, 2017
@jaiminpanchal27 jaiminpanchal27 merged commit 8420558 into prebid:master Oct 30, 2017
Millerrok pushed a commit to Vertamedia/Prebid.js that referenced this pull request Oct 31, 2017
* 'master' of https://github.com/prebid/Prebid.js: (22 commits)
  Update GetIntent adapter to 1.0 version (prebid#1721)
  Add `usePaymentRule` param to AN bidders (prebid#1778)
  New hooks API (replaces monkey-patching for currency) (prebid#1683)
  Change prebidServer to call client user syncs if they exist (prebid#1734)
  Fix Centro adapter to allow requests of the same units (prebid#1746)
  add vastUrl + media type for video bids Prebid Server (prebid#1739)
  Update adxcg adapter for prebid 1.0 (prebid#1741)
  Update yieldmoBid adapter request url (prebid#1771)
  Upgrade Quantcast adapter for Prebid 1.0 (prebid#1753)
  Fidelity Media Adapter update. Prebid v1.0 (prebid#1719)
  Kargo Adapter for Prebid 1.0 (prebid#1729)
  updated for prebid 1.0 api (prebid#1722)
  Add AdOcean adapter (prebid#1735)
  Update Conversant adapter to Prebid 1.0 (prebid#1711)
  Fix test-coverage bug (prebid#1765)
  Migrating TrustX adapter to 1.0 (prebid#1709)
  Update Improve Digital adapter for Prebid 1.0 (prebid#1728)
  Fixed the argument type on getUserSyncs. (prebid#1767)
  nanointeractive bid adapter (prebid#1627)
  Validating bid response params (prebid#1738)
  ...
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request Nov 13, 2017
* tag '0.32.0' of https://github.com/prebid/Prebid.js: (44 commits)
  Prebid 0.32.0 Release
  Commenting out tests that are failing in IE10 (prebid#1710)
  Update dfp.buildVideoUrl to accept adserver url (prebid#1663)
  Update rubicon adapter with new properties and 1.0 changes (prebid#1776)
  Added adUnitCode for compatibility (prebid#1781)
  Remove 'supported' from analytics adapter info (prebid#1780)
  Add TTL parameter to bid (prebid#1784)
  Update GetIntent adapter to 1.0 version (prebid#1721)
  Add `usePaymentRule` param to AN bidders (prebid#1778)
  New hooks API (replaces monkey-patching for currency) (prebid#1683)
  Change prebidServer to call client user syncs if they exist (prebid#1734)
  Fix Centro adapter to allow requests of the same units (prebid#1746)
  add vastUrl + media type for video bids Prebid Server (prebid#1739)
  Update adxcg adapter for prebid 1.0 (prebid#1741)
  Update yieldmoBid adapter request url (prebid#1771)
  Upgrade Quantcast adapter for Prebid 1.0 (prebid#1753)
  Fidelity Media Adapter update. Prebid v1.0 (prebid#1719)
  Kargo Adapter for Prebid 1.0 (prebid#1729)
  updated for prebid 1.0 api (prebid#1722)
  Add AdOcean adapter (prebid#1735)
  ...
@robertrmartinez robertrmartinez deleted the hooks branch July 5, 2023 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants